Issues #8005 and #8926 are both based on an incorrect user belief that an empty method in a Protocol is abstract. However this does not match Python's runtime behavior.

Instead of treating empty methods as abstract, MyPy should flag the method as missing a return statement, and optionally suggest adding the @abstractmethod annotation.

For example, consider this class definition:

class P(Protocol):
    def foo(self) -> int: ...

MyPy should raise an error like this:

error: Missing return statement
note: Did you mean to use @abstractmethod?

First of all, thanks for working on this!

But, sorry, I fail to understand this feature.

Questions I have:

1. I was always sure that we use protocols to define some object's shape, not ever use them to define behavior. So, I was always thinking that all protocol methods are abstract. (Moreover, I think that they should be forced to be empty, but that's a completely different discussion).

That may be how most people think of protocols, but PEP 544 chose to state that they can provide non-abstract “default implementations” and classes can rely on those. Which I think has now led to this confusion about what it means to subclass a proto and how to check it.

So, my goal was to make mypy raise an error for this case:

from typing import Protocol

class Sized(Protocol):
    def __len__(self) -> int: ...

class A(Sized): ...
a = A()
assert isinstance(len(a), int)  # runtime error but no mypy error

The problem here is the interplay of explicit protocol-implementation (i.e., subclassing of Sized) and the possibility of default implementations. Without the possibility of default implementations, there would be no problem. (Because then we could always treat all functions in the protocol as being not-implemented.) But, the PEP does allow for default implementations.

One possible solution would indeed be to force protocol methods to either provide a correct default implementation (i.e., one that returns the correct value) or to be marked as abstract:

from abc import abstractmethod
from typing import Protocol

class P(Protocol):
    def meth1(self) -> int:  # E: Missing return statement
        pass

    @abstractmethod
    def meth2(self) -> int:  # OK
        pass

But I feel that goes a bit against what protocols were meant to do? I feel like default implementations in general are kind of not a good fit for protocols; they're better for abstract base classes, but I don't know. The advantage of this approach is that it works in stub files as well.

The other possibility is to keep track of when empty methods are not overridden, such that when they are called, mypy knows that it can't possibly return the correct type (because empty functions always return None). The disadvantage of this approach is that it doesn't work in stub files where all methods have empty bodies.

This latter approach is kind of what I tried to do here, but I hijacked the abstract-method mechanism to achieve it. Instead of checking at the call site whether a function has an implementation (that has a chance of returning the correct type), I just mark empty functions as abstract, so that mypy can use its normal mechanism to mark methods as non-abstract when an implementation is provided. I guess this is not technically correct, because empty methods in protocols aren't technically abstract, but it does lead to the correct result, that mypy warns when an empty, non-overridden method from a protocol is called.

Hmm, I realize now that mypy in general doesn't complain about empty function bodies: #2350 (there was an attempt to fix it in #8111 but it was abandoned), but I still think the problem is especially bad for protocols, so this PR should be helpful.

I addressed the review feedback.

There is some new relevant discussion in #8926.

Would it be better to remove the check of the return type? This would then be more in line with what pyright does.

Any interest in this? @ilevkivskyi

About special-casing None: I don't think this is a good idea. In general, few people know that protocols (as well as ordinary ABCs) can have default implementations. And even among those who know and use them, few will have empty body as an intended default implementation.

Yeah, that makes sense – I'll remove that special-case-ing.

Also semantic analysis is a wrong place for type-checking. The current implementation already misses couple corner cases, like type variables that can be None, and maybe there are more.

I see. I will try to find the right place.

I think it will be better to give a note that an explicit return None must be used if it is intentional. (IIRC we give instantiation error at type-checking stage, where you can simply use is_subtype())

That sounds good. I will try to figure out how to do that.

@ilevkivskyi I implemented your suggestions (hopefully I understood them correctly), but there is a problem with strict_optional that I don't know how to solve. When strict_optional is False, then is_subtype() is basically useless when the first argument is NoneType. This means, it's not possible to show the helpful note in that case. I guess I could modify is_subtype() so that you can turn off the strict_optional behavior with an additional argument.

The other problem is that the _check_non_ret_from_prot() function is quite complicated and recomputes things that are known at other places in the code, but I think it's probably not worth caching this information from those places, because it's only needed for an error message that should be quite rare.

I'm also confused why I had to add if not typ.is_protocol: in semanal_classprop.py but without that, the stubgen tests were failing. However, it should have been previously already possible to create an abstract protocol by explicitly using @abc.abstractmethod.

I didn't use an Enum for NOT_ABSTRACT/IS_ABSTRACT/IMPLICITLY_ABSTRACT because to my knowledge, enums are not optimized well by mypyc, but I can change that if you want.

EDIT: the error in https://github.com/yelp/paasta seems like a genuine error in their code; I already sent a PR.

I'm also confused why I had to add if not typ.is_protocol: in semanal_classprop.py but without that, the stubgen tests were failing. However, it should have been previously already possible to create an abstract protocol by explicitly using @abc.abstractmethod.

Maybe stubgen automatically adds some imports if it sees an abstract class. In any case, I wouldn't touch semanal_classprop for this, fix the stubgen (or just update the test cases, if there are only few) instead.

I think I addressed everything. The code looks cleaner now, I think.

The only potential problem I see now is the strict_optional issue.

Diff from mypy_primer, showing the effect of this PR on open source code:

paasta (https://github.com/yelp/paasta)
+ paasta_tools/metrics/metrics_lib.py:174: error: Cannot instantiate abstract class "Counter" with abstract attribute "set"